Add test case and proposed fix for path component with trailing colon (and string) #708

jfhamlin · 2018-07-24T18:57:10Z

If a pattern has no verb, match with the verb argument appended to last component.

As I understand the implementation, parsing out a "verb" from the input path is a convenience for matching. Whether the colon and string are actually a verb depends on the matching HTTP rule. This change gives a verb-less pattern a chance to match a path by assuming the colon+string suffix are a part of the final component.

There is a backwards compatibility issue here: this can change the behavior of existing rule sets. For example:

    option(google.api.http) = {
      get: "/users/{user_id}"
    };
    option(google.api.http) = {
      get: "/users/{user_id}:averb"
    };

Before this change, only the second rule matched a request for /users/123:averb. After this change, both rules match (correctly, by my understanding of https://github.com/googleapis/googleapis/blob/master/google/api/http.proto). Note that the spec describes a deterministic way of breaking the tie: // **NOTE:** All service configuration rules follow "last one wins" order. grpc-ecosystem/grpc-gateway's implementation looks buggy on this score, since it accepts the first matching handler.

Fixes #224

See grpc-ecosystem#224 Signed-off-by: James Hamlin <james@goforward.com>

googlebot · 2018-07-24T18:57:15Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

codecov-io · 2018-07-24T19:09:16Z

Codecov Report

Merging #708 into master will increase coverage by <.01%.
The diff coverage is 62.5%.

@@            Coverage Diff             @@
##           master     #708      +/-   ##
==========================================
+ Coverage   56.43%   56.43%   +<.01%     
==========================================
  Files          30       30              
  Lines        3046     3053       +7     
==========================================
+ Hits         1719     1723       +4     
- Misses       1158     1159       +1     
- Partials      169      171       +2

Impacted Files	Coverage Δ
runtime/mux.go	`44.16% <ø> (ø)`	⬆️
runtime/pattern.go	`89.79% <62.5%> (-1.64%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 209d7ce...6d5b579. Read the comment docs.

jfhamlin · 2018-07-24T19:16:38Z

fyi @yugui @tmc

After this change, both rules match (correctly, by my understanding of https://github.com/googleapis/googleapis/blob/master/google/api/http.proto).

I'll provide a little more color on why I think this is so. Here's the relevant section of http.proto:

// The syntax of the path template is as follows:
//
//     Template = "/" Segments [ Verb ] ;
//     Segments = Segment { "/" Segment } ;
//     Segment  = "*" | "**" | LITERAL | Variable ;
//     Variable = "{" FieldPath [ "=" Segments ] "}" ;
//     FieldPath = IDENT { "." IDENT } ;
//     Verb     = ":" LITERAL ;

This is the syntax of templates, not the paths templates are to match. It's actually a template that defines the syntax of the input paths it matches. If a template doesn't include a verb, then any trailing colon and string in an input path must be a part of the final segment.

googlebot · 2018-07-24T19:16:41Z

CLAs look good, thanks!

…onent Parsing out a "verb" from the input path is a convenience for parsing. Whether the colon and string are _actually_ a verb depends on the matching HTTP rule. This change gives a verb-less pattern a chance to match a path by assuming the colon+string suffix are a part of the final component. Fixes grpc-ecosystem#224 Signed-off-by: James Hamlin <james@goforward.com>

jfhamlin · 2018-07-30T20:47:44Z

bump on this (and cc @johanbrandhorst since i've seen you chime in on recent PRs 😃)

achew22 · 2018-07-30T23:41:36Z

runtime/mux_test.go

+				},
+			},
+			reqMethod: "GET",
+			reqPath:   "/foo/bar:123",


Is the verb in this case "123"? What does it mean to have a verb of 123?

Is the verb in this case "123"?

My reading of the HttpRule spec makes me interpret it like this: this path matches the path template in the stub pattern, which has no verb, so "123" isn't a verb (I try to make a case for this reading in #708 (comment)). Instead, "bar:123" is the captured value of the id variable.

To flesh this out further:

An HTTP rule path template describes a set of paths, basically by a regular expression

If a request path is matched by an RPC's HTTP rule (regular expression), then it is handled by that RPC, with fields mapped according to the rule

It follows that if an HTTP rule path template doesn't end with a verb, then it expects no verb, and the trailing path component will be scanned the same way as any other segment or variable

Here's another way to think about it: an alternative implementation of these patterns in grpc-gateway could have been to build up golang regexp.Regexps directly from the HTTP rules. Then when a request comes in, iterate through the regexps and use the last one that matches the path. Example mappings from rule to regular expression ([^/]* probably isn't precisely correct, it's just an approximation of "The syntax * matches a single path segment"):

get: "/foo/{id}" => "/foo/(?P<id>[^/]*)" get: "/foo/{id}:myverb" => "/foo/(?P<id>[^/]*):myverb" get: "/foo/{id}/bar" => "/foo/(?P<id>[^/]*)/bar"

@achew22, does this make sense?

Ah, I see. You are trying to describe the positive case, and not the negative case as I had interpreted it. That seems entirely reasonable. Merging.

Thanks so much for fighting through my confusion and for contributing to the project as a whole!

* Add explicit dependency versions (grpc-ecosystem#696) Version constraints are copied from existing Bazel rules. In the future, these version upgrades must be performed atomically. * Add OpenTracing support to docs (grpc-ecosystem#705) * protoc-gen-swagger: support all well-known wrapper types There were a few well-known wrapper types missing from the wkSchemas map. In specific UInt32Value, UInt64Value and BytesValue. This change handles them (and maps them to the same swagger types as the non-wrapped values) This also fixes the mapping of Int64Value. The Int64Value handling maps the value to a swagger integer. The documentation for Int64Value indicates that it should be mapped to a JSON string (also the mapping for normal int64 in protoc-gen-swagger maps it to a string, so this was inconsistent.) * Add test case and proposed fix for path component with trailing colon (and string) (grpc-ecosystem#708) * If a pattern has no verb, match with a verb arg appended to last component Parsing out a "verb" from the input path is a convenience for parsing. Whether the colon and string are _actually_ a verb depends on the matching HTTP rule. This change gives a verb-less pattern a chance to match a path by assuming the colon+string suffix are a part of the final component. Fixes grpc-ecosystem#224 Signed-off-by: James Hamlin <james@goforward.com> * Fix up examples * Make support paths option Make protoc-gen-grpc-gateway support paths option such like golang/protobuf#515.

kassiansun · 2018-09-10T03:48:07Z

Then, how to gain the old behavior of {id}:verb? Breaking the old behavior without any solution?

kassiansun · 2018-09-10T03:49:11Z

ping @jfhamlin @achew22

kassiansun · 2018-09-10T04:00:21Z

Who can tell me what's the meaning of syntax {id}:verb after this patch, if it matches the whole segment to id?

kassiansun · 2018-09-10T04:56:26Z

Actually it matches to the {id} route, which has no trailing :verb.

And in #224 , I don't think URN is valid inside URL, they are exclusive.

jfhamlin · 2018-09-10T06:45:20Z

Who can tell me what's the meaning of syntax {id}:verb after this patch, if it matches the whole segment to id?

Hi @kassiansun. It would be useful in these examples to not only have the path pattern but also some example paths and expected variable binding. For example:

pattern: /{id}:verb
path: /foo:verb match binding: id = foo
path: /foo no match

pattern: /{id}
path: /foo:verb match binding: id = foo:verb
path: /foo match binding: id = foo

Does that make sense?

kassiansun · 2018-09-10T10:29:47Z

No, the real behavior is /{id}:verb matches /foo:verb with id=foo:verb. Have you tested this patch?

kassiansun · 2018-09-10T10:32:03Z

#224 makes no sense, users should not use : in url as grpc-gateway use : for verb literals. If they really want to use, use url queries, don't mess with path variable.

The solution for #224 turned out to break backwards compatibility, so we're going to have to find another solution for users who desire this behaviour. Also adds test cases from #760.

…lity" This reverts commit 60a328a.

… (and string) (grpc-ecosystem#708) * If a pattern has no verb, match with a verb arg appended to last component Parsing out a "verb" from the input path is a convenience for parsing. Whether the colon and string are _actually_ a verb depends on the matching HTTP rule. This change gives a verb-less pattern a chance to match a path by assuming the colon+string suffix are a part of the final component. Fixes grpc-ecosystem#224 Signed-off-by: James Hamlin <james@goforward.com>

The solution for grpc-ecosystem#224 turned out to break backwards compatibility, so we're going to have to find another solution for users who desire this behaviour. Also adds test cases from grpc-ecosystem#760.

Add test for colon in final path segment not meant as verb

1f98a92

See grpc-ecosystem#224 Signed-off-by: James Hamlin <james@goforward.com>

googlebot added the cla: no label Jul 24, 2018

googlebot added cla: yes and removed cla: no labels Jul 24, 2018

jfhamlin force-pushed the fix/colon-path-segment branch from 31ab2fe to 92019a6 Compare July 24, 2018 19:24

jfhamlin force-pushed the fix/colon-path-segment branch from 92019a6 to 6d5b579 Compare July 24, 2018 19:25

jfhamlin changed the title ~~Add test case and proposed fix for path component with trailing colon~~ Add test case and proposed fix for path component with trailing colon (and string) Jul 24, 2018

achew22 reviewed Jul 30, 2018

View reviewed changes

achew22 merged commit 7226a0d into grpc-ecosystem:master Aug 1, 2018

jfhamlin deleted the fix/colon-path-segment branch August 1, 2018 23:59

johanbrandhorst mentioned this pull request Oct 1, 2018

URLs using verb no longer work after upgrading to v1.5.0 #760

Closed

jfhamlin mentioned this pull request Oct 1, 2018

404s using colons in the middle of the last path segment #224

Closed

jfhamlin added a commit to jfhamlin/grpc-gateway that referenced this pull request Jun 15, 2019

Revert "Revert grpc-ecosystem#708 since it breaks backwards compatibi…

f676703

…lity" This reverts commit 60a328a.

jfhamlin mentioned this pull request Jun 15, 2019

Support colon in final path segment, last match wins behavior (behind flags) #949

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test case and proposed fix for path component with trailing colon (and string) #708

Add test case and proposed fix for path component with trailing colon (and string) #708

jfhamlin commented Jul 24, 2018

googlebot commented Jul 24, 2018

codecov-io commented Jul 24, 2018 •

edited

Loading

jfhamlin commented Jul 24, 2018

googlebot commented Jul 24, 2018

jfhamlin commented Jul 30, 2018

achew22 Jul 30, 2018

jfhamlin Jul 31, 2018 •

edited

Loading

jfhamlin Aug 1, 2018

achew22 Aug 1, 2018

kassiansun commented Sep 10, 2018

kassiansun commented Sep 10, 2018

kassiansun commented Sep 10, 2018

kassiansun commented Sep 10, 2018

jfhamlin commented Sep 10, 2018

kassiansun commented Sep 10, 2018

kassiansun commented Sep 10, 2018

Add test case and proposed fix for path component with trailing colon (and string) #708

Add test case and proposed fix for path component with trailing colon (and string) #708

Conversation

jfhamlin commented Jul 24, 2018

googlebot commented Jul 24, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

codecov-io commented Jul 24, 2018 • edited Loading

Codecov Report

jfhamlin commented Jul 24, 2018

googlebot commented Jul 24, 2018

jfhamlin commented Jul 30, 2018

achew22 Jul 30, 2018

Choose a reason for hiding this comment

jfhamlin Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

jfhamlin Aug 1, 2018

Choose a reason for hiding this comment

achew22 Aug 1, 2018

Choose a reason for hiding this comment

kassiansun commented Sep 10, 2018

kassiansun commented Sep 10, 2018

kassiansun commented Sep 10, 2018

kassiansun commented Sep 10, 2018

jfhamlin commented Sep 10, 2018

kassiansun commented Sep 10, 2018

kassiansun commented Sep 10, 2018

codecov-io commented Jul 24, 2018 •

edited

Loading

jfhamlin Jul 31, 2018 •

edited

Loading